fix(checkout): count repo failures and disjoin them into the exit code - #903
Conversation
laynepenney
left a comment
laynepenney
left a comment
There was a problem hiding this comment.
ATLAS r2 BLOCK, merge gate, bound to exact head f99f1edac857037b0e697218b284de7c0478a740.
Frozen artifact bind, independently re-derived:
range.patchRAW:7549911bfa5ef0c8ae0eb66e7e8b90212b05cb8d25c7038e7c8570b00eab1a12metadata.fuller.txtRAW:c37f087535227342c7560f5c7c3ad816542ebcad13079213ca490096e8e21384pr-title.txtNORM:673164cdbfcbf00c029e3ca60adf7c8aab7e73092d13c3da1cf6c384a7e2da44pr-body.mdNORM:0555cf5061d4c9a0579af3a3ab039c3f1881adea9c97949c7751d500a90b220b
BLOCKERS
-
cargo fmt --checkfails on two files changed by this head:src/cli/commands/checkout.rs: import orderingtests/test_checkout.rs: the long assertion needs the multiline form required by rustfmt
I ran the same command on exact base
1dd7474c822c993345bd3a5f7111b1806b383881as a control. Base has one unrelated pre-existing failure intests/manifest_input_warnings.rs; the two failures above are introduced by this range. -
The frozen commit message and PR body say the two test fixtures "differ only in whether
.gitexists." Literally, the complete fixtures also differ in repository count/state and branch. The tests do make the intended distinction, so narrow the claim to what the evidence establishes: absent.gitskips successfully, while present-but-unopenable.gitis counted as an error and returns nonzero.
CALLS ON THE THREE REQUESTED SEAMS
-
PASS:
RepoInfo::exists()is the right cloned predicate here. It matches the helper contract and checkout behavior before adopting the helper. The majority use of directory existence by unrelated callers does not override this command-specific invariant. The helper currently has no other callers. -
PASS: plain
anyhow::bail!is correct. This is failure to perform, exit 1, not understood-and-refused, exit 2. The per-repo messages explain individual failures; the aggregate error count is not otherwise rendered, so treating the aggregate as unreported is also correct. -
PASS functionally: the pair discriminates. Replacing
repo.exists()with directorypath_existskilled only the non-git skip witness. Removing the error-count exit disjunction killed only the all-repositories-failing witness. The wording still needs the narrowing above.
RAN:
- all four hash derivations
- live remote head/base checks with present and absent controls
- ahead/behind: 1/0
- regenerated range patch and metadata, byte-identical to frozen artifacts
git diff --check: exit 0- focused checkout suite: 12/12
- full
cargo test: exit 0 - both targeted mutants described above
cargo fmt --checkat head: exit 1cargo fmt --checkat base control: exit 1 only for the unrelated pre-existing file
READ:
- every line of the four frozen artifacts
- live PR title/body against the frozen platform text
- surrounding
RepoInfo::exists, outcome classification, main renderer, checkout, helper, tests, and release caller
Format the changed files and correct the frozen prose, then amend and re-freeze. The new head and body need a fresh r2.
gr checkout reported every per-repo failure to the terminal and incremented no counter, so a run in which every repo failed printed "Switched 0/N repos to <branch>" and exited 0. The printed ratio is something a caller has to read and interpret; the exit code is the only failure signal a script sees, and it reported the batch as done. Adopt cli/repo_iter::for_each_repo -- which is what gives this command an error count at all -- and make a nonzero exit disjoin the per-repo failures. Two changes fall out of the adoption: - for_each_repo's cloned-check called path_exists on the repo directory while its own docstring promised to skip repos that "aren't cloned". RepoInfo::exists tests for .git, which is what cloned means; the two diverge on a directory that exists and is not a clone. Aligning the code with the docstring preserves checkout's existing behavior exactly rather than reclassifying a skip as an error. - for_each_repo_path had no callers and gains none here, so it is removed rather than left relying on pub visibility to keep its own dead-code warning quiet. The plan step prescribing that suppression is struck in the same change. An unused private item warns and an unused pub item does not, so following the step as written left the module with zero callers and nothing anywhere going red for as long as it existed. A warning is a detector; silencing one to reach a clean build is not a fix. Tests pin both sides of one property. The discriminator is an ABSENT .git versus a PRESENT BUT UNOPENABLE one: absent is a skip and still exits 0, present but unopenable is an error and exits nonzero. Without both sides, a witness asserting only the failure case could pass while the skip path had silently become an error too. These are two separate tests with their own fixtures, and those fixtures differ in more than the discriminating property -- one corrupts a single repo and the other both, one creates a branch first, and they target different branches. The claim is about which property discriminates, not about the fixtures being otherwise identical.
f99f1ed to
a9263bf
Compare
laynepenney
left a comment
laynepenney
left a comment
There was a problem hiding this comment.
ATLAS r2 APPROVE, merge gate, bound to exact head a9263bf1bc12532bad1c1199946255b259f42315.
Frozen v3 binds, independently re-derived:
range.patchRAW:7cec0eae17ee6877787254c9eb3356686f7c3b9917fde9dafa7e505b2bc5b383metadata.fuller.txtRAW:9df6c90c895aca999dab9b66d2808299626a6b1443f9d9b3f5f7262bce988c3apr-title.txtNORM:673164cdbfcbf00c029e3ca60adf7c8aab7e73092d13c3da1cf6c384a7e2da44pr-body.mdNORM:626e7a09afadbe3d331cf942eb593b4a3d6252fdadc7364bd57652647aeb4ef6
Both prior blockers are closed.
-
Formatting: the two PR-changed files pass
rustfmt --check --edition 2021. I rancargo fmt --checkat both exact base1dd7474c822c993345bd3a5f7111b1806b383881and this head. Both return nonzero only for the same pre-existingtests/manifest_input_warnings.rs:281delta. The PR introduces no formatting delta and correctly leaves that unrelated file outside this range. -
Difference-set claim: the commit message and live PR body now distinguish the property under test from fixture identity. They name absent
.gitversus present-but-unopenable.git, state that the fixtures are not otherwise identical, and enumerate the other material differences. The retireddiffer onlyclass is absent from the frozen and live surfaces.
The three requested calls remain:
RepoInfo::exists(): PASS.- Plain
anyhow::bail!: PASS. - Test pair: PASS and mutation-discriminating.
RAN at exact v3 head:
- all four frozen hash derivations
- live remote base/head plus present and absent controls
- ahead/behind: 1/0
- regenerated range patch and fuller metadata, byte-identical to frozen artifacts
- live title/body normalized round-trip, both equal frozen
- retired-phrase negative scan plus replacement-text positive control
git diff --check: exit 0- changed-file
rustfmt --check --edition 2021: exit 0 - base/head
cargo fmt --checkdifferential control described above - focused checkout suite: 12/12
- full
cargo test --quiet: exit 0 - cloned-predicate mutant: skip witness failed, all-failing witness passed
- exit-disjunction mutant: all-failing witness failed, skip witness passed
- restored isolated tree: clean, exact head retained
READ:
- every line of the five-file v3 range, fuller metadata, title, and body
- complete v2 to v3 range, metadata, and platform-text delta
- the repaired formatting locations and complete difference-set wording
- surrounding cloned predicate, outcome classification, rendering, checkout, helper, tests, and release caller
Scope: this APPROVE closes r2 for the merge gate at the exact head above. Stromus's separate r1 covers the public-push gate.
1 participant
What
gr checkoutreported each per-repo failure to the terminal and incremented no counter, so a run in which every repo failed printedSwitched 0/N repos to <branch>and exited 0. The printed ratio is something a caller has to read and interpret; the exit code is the only failure signal a script or CI job sees, and it reported the batch as done.This adopts
cli/repo_iter::for_each_repo— which is what supplies an error count at all — and makes a nonzero exit disjoin the per-repo failures.Behavior change
gr checkoutnow exits nonzero when one or more repos fail to check out. A repo where the target branch simply does not exist is a skip, not a failure, and still exits 0.This is a deliberate change to a shipped exit code, called out here so it is not discovered downstream. Nothing documented the previous behavior; it fell out of an unconditional
Ok(()). A script that depends on the old exit 0 is a script that is currently green over failed checkouts.Two changes that fall out of the adoption
for_each_repo. It calledpath_existson the repo directory, while its own docstring promised to skip repos that "aren't cloned".RepoInfo::existstests for.git, which is what cloned means. The two diverge on a directory that exists and is not a clone. Aligning the code with the docstring preservescheckout's existing behavior exactly, rather than quietly reclassifying an existing skip as an error. The mismatch had never been exposed because the helper had no callers.for_each_repo_pathis removed. It had no callers and gains none here.Plan-doc correction
docs/PLAN-p2-maintainability.mdcarried a step saying that if clippy flagged unusedrepo_iterimports, thepubvisibility fromcli/mod.rsshould suppress it. That step is struck. An unused private item warns and an unusedpubitem does not — so following the step as written left the module with zero callers and nothing anywhere going red for as long as it existed. A warning is a detector; silencing one to reach a clean build is not a fix, and writing the suppression down as a step made it the default for whoever came next.The same doc's scoped-out row said most commands "accumulate custom state that doesn't fit the simple Success/Skipped/Error enum". That is largely right, and it is now measured rather than assumed: of the nine command files that hand-roll these counters, two do not iterate repos at all, one iterates pull requests asynchronously, and three propagate
?out of the loop — which the closure's return type cannot express.checkoutwas the one clean fit.Tests
Two tests pin both sides of one property. The discriminator is an absent
.gitversus a present but unopenable one:.gitremoved → not cloned → skip → exits 0 (the existingtest_checkout_skips_non_git_repo).gitpresent but not a valid repository → error → exits nonzero (new)These are separate tests with their own fixtures, and those fixtures differ in more than the discriminating property: one corrupts a single repo and the other both, one creates a branch first, and they target different branches. The claim is about which property discriminates — verified by mutation — not about the fixtures being otherwise identical.
Without both sides, a witness asserting only the failure case could pass while the skip path had silently become an error as well. If the first case ever goes red the pair stops discriminating and the second proves nothing; that reasoning is recorded in the test itself.
A third test pins that an absent branch remains a skip, so the change cannot over-correct into failing runs that are legitimately no-ops.
Full test suite green.
Why
anyhow::bail!and not thecli/outcome.rsvocabularyCliOutcomeErrorexists and this change deliberately does not use it. Two separate questions sit behind that:The exit code.
exit_code_for_errorfalls back to1for a plain error, which is the right code here.EXIT_REFUSED(2) means the command understood the request and declined to act; a repo that failed to check out is a failure to perform, not a refusal. Giving both the same code would make an upstream failure indistinguishable from a deliberate one, and the distinction is the reason that module exists.The rendering.
error_was_reporteddefines itself as whether the command already printed the diagnostic that explains this failure. The per-repo failures are printed inside the loop, and each explains one repo. The error returned here is the aggregate, and its count is printed nowhere else:Switched 3/5 repos to <branch>reports successes and does not distinguish a repo that failed from one that was skipped. So the summary linemainrenders is the only place the failure count appears, and marking this error already-reported would delete it.Worth stating plainly, since it is the same shape as the defect above: that rendering currently comes from
unwrap_or(false)rather than from anything this code declares. The behavior is right and it is inherited from a default rather than asserted. Adding a constructor to express it would put new surface inoutcome.rswith exactly one caller, which is the trade this change is not worth making.Premium boundary
Premium boundary:
gripis OSS because this is workspace orchestration and CLI process semantics — no identity, no organization state, no entitlement behavior.Ref #886 — this implements rule 3 at one call site and does not close the contract issue.